If we encounter a standard that requires us to check multiple policy fields at once, we could add a more generic validate_policy function

I wonder if another option could be to just pass in the security policy to each of these validate functions. That way each function still returns whether or not a given item is valid, it just has the option to check other security policy fields to make that decision.

Ultimately I want to use these for more than just security policies. For example, I want to check the actual security parameters chosen by the connection for FIPS compliance.

If we run into a rule that requires checking multiple policy fields, we can reconsider. But I suspect that won't be common, so I'd like to try to keep these simple.

Like, if you have a validate_cipher_suite(cipher, is_valid) method, it's going to be doing pretty simple checks and we're probably fine with the common security rule code handling logging the error to output just based on "is_valid". If you're going to do more complex checks though, we'll want to pass the result object too so that you can do your own logging, and now your implementation of your rule is getting more complicated :)

Do we think these callbacks are going to scale with more complex usecases? For example, let's say a ciphersuite should only be used with a specific chosen version of TLS. Or a ciphersuite and curve should be chosen together.

No, but I don't think that's an issue.

Both your hypotheticals are only relevant to checking a connection / handshake, not a security policy. A security policy can't assume which version of TLS will be chosen (it can check the policy's min version, but that's not really sufficient) or guarantee that two options (a cipher suite and a curve) will be chosen together. Like I call out in the description, we'd probably want a validate_handshake(struct s2n_connection *conn, struct s2n_security_rule_result *result) method to cover cases like that when specifically checking the actual negotiated parameters instead of a security policy. We also don't have any cases for checks like that yet-- both FIPS and RFC5191 don't need that. And even if they did, they'd likely still rely heavily on much simpler checks that can be more clearly implemented with these simple callbacks.

IIRC all of the stuffer functions have CBMC proofs. It's probably good to have some for these new functions as well.

Definitely agree, but I think getting CBMC to work with varargs is going to be a bit of an adventure. I'd rather that didn't block this PR, particularly because the only direct manipulation of the stuffer is the "tainted" flag logic. Other than that, we just use existing stuffer functions.

I would prefer to invert this logic and have something like bool is_valid, bool passed or bool success.

The trick there is that we then need to initialize is_valid to "true" before calling any of our validation methods. It'll default to incorrectly start as "false". If we use the negative case, it starts out correct and methods just need to set it if they find an issue. Like, our checks don't ever definitively mark a policy as "valid", they only ever flag it as "not valid", and if they never flag it then it's valid.

Is this field needed if you just key'ed on the stuffer being initialized or not?

I started out doing that, but it's a little messy. Either we assume the stuffer is always growable and check "output->growable", or we need a more complex check like "!s2n_stuffer_is_freed(output) || output->growable".

I like the flag because it explicitly indicates that we expect output. If the stuffer isn't configured properly but the flag is set, we'll fail. Otherwise, if the stuffer isn't configured properly, we just skip output.

Fair enough

Do we think these callbacks are going to scale with more complex usecases? For example, let's say a ciphersuite should only be used with a specific chosen version of TLS. Or a ciphersuite and curve should be chosen together.